feat(inspector): attach Chrome DevTools to Web Worker isolates#1973
Open
edusperoni wants to merge 2 commits into
Open
feat(inspector): attach Chrome DevTools to Web Worker isolates#1973edusperoni wants to merge 2 commits into
edusperoni wants to merge 2 commits into
Conversation
Workers were invisible to the debugger: the inspector only served the
main isolate and even dropped worker console output on the floor.
DevTools discovers extra isolates through the Target domain with
flat-session multiplexing, so implement that surface in the runtime:
- Add WorkerInspectorClient: a per-worker V8Inspector + session that
lives entirely on the worker's thread. Incoming CDP messages queue
through an eventfd on the worker's ALooper (the same mechanism as the
worker message inbox); while paused at a breakpoint a nested loop on
the worker thread pumps the same queue without re-entering the
looper, so postMessage deliveries stay queued during a pause,
matching Chrome's semantics.
- Turn JsV8InspectorClient into the root session + router: handle
Target.setAutoAttach natively on the socket thread (announce workers
via Target.attachedToTarget, detach on death), route messages
carrying a top-level sessionId to the worker's thread directly from
the socket thread, and make the frontend sender thread-safe. Routing
off the socket thread means a worker stays debuggable while the main
isolate is paused, and vice versa.
- Debugger.pause for a busy worker uses RequestInterrupt keyed by
workerId, so a late-firing interrupt after worker death is a no-op;
it is skipped while that worker is already paused, avoiding a
spurious re-pause after resume (same guard as the main isolate).
- Serve Network.loadNetworkResource and IO.read/IO.close on the socket
thread for any session (sessionId echoed), so worker source maps
load too, and apply the nsruntime:// sourceMapURL rewrite to worker
Debugger.scriptParsed events.
- Route worker console.* to the worker's own inspector and deliver
through V8ConsoleMessageStorage::addMessage instead of the live-only
runtime agent path: messages logged before the frontend attaches are
stored and replayed as console history. Applies to the main isolate
as well.
- Name execution contexts ("main" / worker script url) so the DevTools
console context selector rows are labeled and selectable.
- Worker lifecycle: the inspector is created on the worker thread
before the script runs (debug builds only), terminate() kicks a
paused worker out of its nested pause loop, teardown unregisters the
target before the isolate is disposed, and frontend reconnects reset
worker sessions (ordered before the main-isolate Locker so workers
recover even if the main isolate is paused).
- Make isConnected_ atomic: worker threads now read it while the main
thread writes the adjacent bitfield flags.
waitForDebuggerOnStart (pause new workers before their first line) is
left as future work; workers announce waitingForDebugger: false.
Ports NativeScript/ios#386 to Android.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR adds worker thread debugging support to the Android V8 inspector. The socket message handler contract shifts from string return to boolean-with-response, worker targets are registered and routed via session identifiers, a new per-worker inspector client integrates with the worker's event loop, and inspector lifecycle is wired into worker bootstrap/teardown with console message forwarding. ChangesWorker Thread Debugging Infrastructure
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
test-app/runtime/src/main/cpp/JsV8InspectorClient.cpp (1)
514-524: 🩺 Stability & Availability | 🏗️ Heavy liftAvoid socket writes while holding
workerTargetsMutex_.Line 514+ / Line 542+ / Line 571+ / Line 585+ call
SendToFrontend(...)while the registry mutex is held. If socket send blocks, worker register/unregister/routing can stall behind that lock. Snapshot payloads under lock, then send after unlocking.Also applies to: 542-559, 571-577, 585-605
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test-app/runtime/src/main/cpp/JsV8InspectorClient.cpp` around lines 514 - 524, You're holding workerTargetsMutex_ while performing socket writes via SendToFrontend/JsonDump (e.g., in the branch handling missing sessionId and other branches that currently call SendToFrontend while locked), which can block other registry operations; instead, inside the critical section (while holding workerTargetsMutex_) construct and copy the JSON payload/string you intend to send and then release the lock, and only after unlocking call SendToFrontend(JsonDump(...)) or SendToFrontend(copiedPayload). Apply this pattern for all occurrences that call SendToFrontend while workerTargetsMutex_ is held (search for SendToFrontend usages near workerTargets_ handling: the missing-session branch, and the other locations noted in the comment).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test-app/runtime/src/main/cpp/JsV8InspectorClient.cpp`:
- Around line 814-821: GetInstance currently does an atomic load->new->store
which can race and leak if two threads concurrently see null; change
JsV8InspectorClient::GetInstance to perform a race-free singleton init by using
either std::call_once with a std::once_flag or an atomic compare_exchange loop
on the static instance so only one caller constructs the object and others reuse
it; keep the creation using Runtime::GetRuntime(0)->GetIsolate() and ensure
GetInstanceIfCreated remains unchanged, and verify callers like
Java_com_tns_AndroidJsV8Inspector_handleMessageOnSocketThread use the fixed
GetInstance to avoid double construction.
In `@test-app/runtime/src/main/cpp/WorkerInspectorClient.cpp`:
- Around line 205-239: The pauseTerminated_ flag is subject to a data race
because it is written from NotifyTerminating() (called from another thread)
while being read/modified in runMessageLoopOnPause() and
quitMessageLoopOnPause(); change the pauseTerminated_ member in
WorkerInspectorClient to std::atomic<bool> and update all accesses to use atomic
operations with appropriate memory ordering (e.g., load(memory_order_acquire)
when reading in runMessageLoopOnPause and store(memory_order_release) when
setting in NotifyTerminating and quitMessageLoopOnPause) so reads/writes are
synchronized across threads.
---
Nitpick comments:
In `@test-app/runtime/src/main/cpp/JsV8InspectorClient.cpp`:
- Around line 514-524: You're holding workerTargetsMutex_ while performing
socket writes via SendToFrontend/JsonDump (e.g., in the branch handling missing
sessionId and other branches that currently call SendToFrontend while locked),
which can block other registry operations; instead, inside the critical section
(while holding workerTargetsMutex_) construct and copy the JSON payload/string
you intend to send and then release the lock, and only after unlocking call
SendToFrontend(JsonDump(...)) or SendToFrontend(copiedPayload). Apply this
pattern for all occurrences that call SendToFrontend while workerTargetsMutex_
is held (search for SendToFrontend usages near workerTargets_ handling: the
missing-session branch, and the other locations noted in the comment).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a62cec75-6809-44d1-af66-cbcd3dff76e0
📒 Files selected for processing (9)
test-app/app/src/main/java/com/tns/AndroidJsV8Inspector.javatest-app/runtime/CMakeLists.txttest-app/runtime/src/main/cpp/JsV8InspectorClient.cpptest-app/runtime/src/main/cpp/JsV8InspectorClient.htest-app/runtime/src/main/cpp/WorkerInspectorClient.cpptest-app/runtime/src/main/cpp/WorkerInspectorClient.htest-app/runtime/src/main/cpp/WorkerWrapper.cpptest-app/runtime/src/main/cpp/WorkerWrapper.htest-app/runtime/src/main/cpp/com_tns_AndroidJsV8Inspector.cpp
Comment on lines
+205
to
+239
| void WorkerInspectorClient::runMessageLoopOnPause(int contextGroupId) { | ||
| if (runningPauseLoop_.load(std::memory_order_acquire) || dying_) { | ||
| return; | ||
| } | ||
| runningPauseLoop_.store(true, std::memory_order_release); | ||
| pauseTerminated_ = false; | ||
|
|
||
| while (!pauseTerminated_ && !dying_) { | ||
| std::string message = this->PopMessage(); | ||
| bool shouldWait = message.empty(); | ||
| if (!shouldWait) { | ||
| this->DispatchOne(message); | ||
| } | ||
|
|
||
| while (v8::platform::PumpMessageLoop(Runtime::platform, isolate_)) { | ||
| } | ||
|
|
||
| if (shouldWait && !pauseTerminated_ && !dying_) { | ||
| std::unique_lock<std::mutex> lock(messageArrivedMutex_); | ||
| messageArrived_.wait_for(lock, std::chrono::milliseconds(1)); | ||
| } | ||
| } | ||
|
|
||
| runningPauseLoop_.store(false, std::memory_order_release); | ||
| } | ||
|
|
||
| void WorkerInspectorClient::quitMessageLoopOnPause() { | ||
| pauseTerminated_ = true; | ||
| } | ||
|
|
||
| void WorkerInspectorClient::NotifyTerminating() { | ||
| dying_ = true; | ||
| pauseTerminated_ = true; | ||
| messageArrived_.notify_all(); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Data race on pauseTerminated_ flag.
pauseTerminated_ is written from a different thread in NotifyTerminating() (called via WorkerWrapper::Terminate() from the parent thread) while it may be read concurrently in runMessageLoopOnPause() on the worker thread. This needs to be an std::atomic<bool> with appropriate memory ordering.
🔒 Proposed fix: make pauseTerminated_ atomic
In the header, change:
- bool pauseTerminated_ = false;
+ std::atomic<bool> pauseTerminated_{false};Then update usages with appropriate memory ordering:
void WorkerInspectorClient::runMessageLoopOnPause(int contextGroupId) {
if (runningPauseLoop_.load(std::memory_order_acquire) || dying_) {
return;
}
runningPauseLoop_.store(true, std::memory_order_release);
- pauseTerminated_ = false;
+ pauseTerminated_.store(false, std::memory_order_relaxed);
- while (!pauseTerminated_ && !dying_) {
+ while (!pauseTerminated_.load(std::memory_order_acquire) && !dying_) {
// ...
- if (shouldWait && !pauseTerminated_ && !dying_) {
+ if (shouldWait && !pauseTerminated_.load(std::memory_order_acquire) && !dying_) {
// ...
}
}
// ...
}
void WorkerInspectorClient::quitMessageLoopOnPause() {
- pauseTerminated_ = true;
+ pauseTerminated_.store(true, std::memory_order_release);
}
void WorkerInspectorClient::NotifyTerminating() {
dying_ = true;
- pauseTerminated_ = true;
+ pauseTerminated_.store(true, std::memory_order_release);
messageArrived_.notify_all();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| void WorkerInspectorClient::runMessageLoopOnPause(int contextGroupId) { | |
| if (runningPauseLoop_.load(std::memory_order_acquire) || dying_) { | |
| return; | |
| } | |
| runningPauseLoop_.store(true, std::memory_order_release); | |
| pauseTerminated_ = false; | |
| while (!pauseTerminated_ && !dying_) { | |
| std::string message = this->PopMessage(); | |
| bool shouldWait = message.empty(); | |
| if (!shouldWait) { | |
| this->DispatchOne(message); | |
| } | |
| while (v8::platform::PumpMessageLoop(Runtime::platform, isolate_)) { | |
| } | |
| if (shouldWait && !pauseTerminated_ && !dying_) { | |
| std::unique_lock<std::mutex> lock(messageArrivedMutex_); | |
| messageArrived_.wait_for(lock, std::chrono::milliseconds(1)); | |
| } | |
| } | |
| runningPauseLoop_.store(false, std::memory_order_release); | |
| } | |
| void WorkerInspectorClient::quitMessageLoopOnPause() { | |
| pauseTerminated_ = true; | |
| } | |
| void WorkerInspectorClient::NotifyTerminating() { | |
| dying_ = true; | |
| pauseTerminated_ = true; | |
| messageArrived_.notify_all(); | |
| } | |
| void WorkerInspectorClient::runMessageLoopOnPause(int contextGroupId) { | |
| if (runningPauseLoop_.load(std::memory_order_acquire) || dying_) { | |
| return; | |
| } | |
| runningPauseLoop_.store(true, std::memory_order_release); | |
| pauseTerminated_.store(false, std::memory_order_relaxed); | |
| while (!pauseTerminated_.load(std::memory_order_acquire) && !dying_) { | |
| std::string message = this->PopMessage(); | |
| bool shouldWait = message.empty(); | |
| if (!shouldWait) { | |
| this->DispatchOne(message); | |
| } | |
| while (v8::platform::PumpMessageLoop(Runtime::platform, isolate_)) { | |
| } | |
| if (shouldWait && !pauseTerminated_.load(std::memory_order_acquire) && !dying_) { | |
| std::unique_lock<std::mutex> lock(messageArrivedMutex_); | |
| messageArrived_.wait_for(lock, std::chrono::milliseconds(1)); | |
| } | |
| } | |
| runningPauseLoop_.store(false, std::memory_order_release); | |
| } | |
| void WorkerInspectorClient::quitMessageLoopOnPause() { | |
| pauseTerminated_.store(true, std::memory_order_release); | |
| } | |
| void WorkerInspectorClient::NotifyTerminating() { | |
| dying_ = true; | |
| pauseTerminated_.store(true, std::memory_order_release); | |
| messageArrived_.notify_all(); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test-app/runtime/src/main/cpp/WorkerInspectorClient.cpp` around lines 205 -
239, The pauseTerminated_ flag is subject to a data race because it is written
from NotifyTerminating() (called from another thread) while being read/modified
in runMessageLoopOnPause() and quitMessageLoopOnPause(); change the
pauseTerminated_ member in WorkerInspectorClient to std::atomic<bool> and update
all accesses to use atomic operations with appropriate memory ordering (e.g.,
load(memory_order_acquire) when reading in runMessageLoopOnPause and
store(memory_order_release) when setting in NotifyTerminating and
quitMessageLoopOnPause) so reads/writes are synchronized across threads.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Workers were invisible to the debugger: the inspector only served the main isolate and even dropped worker console output on the floor. DevTools discovers extra isolates through the Target domain with flat-session multiplexing, so implement that surface in the runtime:
waitForDebuggerOnStart (pause new workers before their first line) is left as future work; workers announce waitingForDebugger: false.
Ports NativeScript/ios#386 to Android.
Summary by CodeRabbit